Add Name and FileName to IFormFile #500

khellang · 2015-12-04T11:21:23Z

Fixes #499. These names should probably be cached somewhere. Anyone opposed to exposing a Name property on IFormFile backed by a Lazy<string>?

davidfowl · 2015-12-04T11:23:01Z

This isn't how I think it should be done. We should update FormFile to have a name property on it. Make it lazy there.

khellang · 2015-12-04T11:24:24Z

@davidfowl Hence my "These names should probably be cached somewhere. Anyone opposed to exposing a Name property on IFormFile backed by a Lazy<string>?" 😉

davidfowl · 2015-12-04T11:25:35Z

https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNet.Http/Features/FormFeature.cs#L150-L160

davidfowl · 2015-12-04T11:26:17Z

@khellang Lazy is overkill, it doesn't need to be thread safe. Also add a test for this once you're done.

khellang · 2015-12-04T11:29:05Z

Lazy is overkill, it doesn't need to be thread safe.

So are you saying parse the name eagerly? Or just _name ?? (_name = GetName())?

davidfowl · 2015-12-04T11:38:48Z

Lazy is fine but using Lazy<T> is just overhead since we don't care about thread safety.

khellang · 2015-12-04T11:40:27Z

@davidfowl How does it look now?

davidfowl · 2015-12-04T11:42:18Z

src/Microsoft.AspNet.Http/Features/FormFeature.cs

@@ -149,7 +149,9 @@ public Task<IFormCollection> ReadFormAsync(CancellationToken cancellationToken)
                            // Find the end
                            await section.Body.DrainAsync(cancellationToken);

-                            var file = new FormFile(_request.Body, section.BaseStreamOffset.Value, section.Body.Length)
+                            var name = HeaderUtilities.RemoveQuotes(contentDisposition.Name);


Can name ever be null here?

davidfowl · 2015-12-04T11:43:47Z

Much better!

khellang · 2015-12-04T11:43:59Z

What about FileName as well?

davidfowl · 2015-12-04T11:44:28Z

Go for it

davidfowl · 2015-12-04T11:56:54Z

I think I change my mind on the lazy. Not worth it. Parse it out and just assign eagerly.

khellang · 2015-12-04T12:01:19Z

Have you looked at the getters for Name and FileName in CDHV? Granted, it's not often you send files, and not very many of them, so it probably won't matter much.

Want me to strip off that last commit then? 😄

khellang · 2015-12-04T12:27:24Z

Added dfca389 to close #352 as well.

davidfowl · 2015-12-04T12:39:24Z

src/Microsoft.AspNet.Http/Features/FormFile.cs


-        public FormFile(Stream baseStream, long baseStreamOffset, long length)
+        public FormFile(Stream baseStream, long baseStreamOffset, long length, ContentDispositionHeaderValue contentDisposition)


Can we move the quote removal on to the outside and just pass the name and file name. It's weird to have the header dependency in here (always was)

Tratcher · 2015-12-04T14:19:14Z

src/Microsoft.AspNet.Http/FormFileCollection.cs

-            return HeaderUtilities.RemoveQuotes(cd?.Name);
+            foreach (var file in this)
+            {
+                if (string.Equals(name, file.Name))


Ordinal or ignore case?

Good question. I just kept what was already there. What would you expect?

OrdinalIgnoreCase

davidfowl · 2015-12-04T18:58:36Z

Ok this PR is good to go once the CI passes. Then we'll have to see what repos break. If I had to guess. MVC would be broken

This commits also gets rid of the name closure in FormFileCollection by interating over the files in the collection instead of using Find and FindAll. Closes aspnet#352 and aspnet#499

Add Name and FileName to IFormFile

dnfclas added the cla-already-signed label Dec 4, 2015

khellang mentioned this pull request Dec 4, 2015

No easy way to get file names in IFormFileCollection / IFormFile #499

Closed

khellang force-pushed the form-file-name branch from 5445250 to c230ca9 Compare December 4, 2015 11:22

khellang force-pushed the form-file-name branch from c230ca9 to feb80f0 Compare December 4, 2015 11:23

khellang force-pushed the form-file-name branch from feb80f0 to 746f121 Compare December 4, 2015 11:37

davidfowl reviewed Dec 4, 2015
View reviewed changes

khellang force-pushed the form-file-name branch from cf081cd to 91a98a8 Compare December 4, 2015 11:59

khellang mentioned this pull request Dec 4, 2015

Possible FormFileCollection optimizations #352

Closed

khellang changed the title ~~Expose GetName extension on IFormFile~~ Add Name and FileName to IFormFile Dec 4, 2015

khellang force-pushed the form-file-name branch 2 times, most recently from 7a3310e to f8bb6f6 Compare December 4, 2015 12:12

davidfowl reviewed Dec 4, 2015
View reviewed changes

khellang force-pushed the form-file-name branch from 5d1cd10 to dfca389 Compare December 4, 2015 12:47

Tratcher reviewed Dec 4, 2015
View reviewed changes

Added Name and FileName to IFormFile

c2e7618

This commits also gets rid of the name closure in FormFileCollection by interating over the files in the collection instead of using Find and FindAll. Closes aspnet#352 and aspnet#499

khellang force-pushed the form-file-name branch from 0ef9636 to c2e7618 Compare December 4, 2015 19:30

khellang mentioned this pull request Dec 4, 2015

React to changes to FormFile aspnet/Mvc#3690

Merged

davidfowl added a commit that referenced this pull request Dec 4, 2015

Merge pull request #500 from khellang/form-file-name

dfe2d41

Add Name and FileName to IFormFile

davidfowl merged commit dfe2d41 into aspnet:dev Dec 4, 2015

khellang deleted the form-file-name branch December 4, 2015 20:04

khellang mentioned this pull request Dec 9, 2015

FormFile missing FileName/Name #505

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Name and FileName to IFormFile #500

Add Name and FileName to IFormFile #500

khellang commented Dec 4, 2015

davidfowl commented Dec 4, 2015

khellang commented Dec 4, 2015

davidfowl commented Dec 4, 2015

davidfowl commented Dec 4, 2015

khellang commented Dec 4, 2015

davidfowl commented Dec 4, 2015

khellang commented Dec 4, 2015

davidfowl Dec 4, 2015

davidfowl Dec 4, 2015

davidfowl commented Dec 4, 2015

khellang commented Dec 4, 2015

davidfowl commented Dec 4, 2015

davidfowl commented Dec 4, 2015

khellang commented Dec 4, 2015

khellang commented Dec 4, 2015

davidfowl Dec 4, 2015

khellang Dec 4, 2015

Tratcher Dec 4, 2015

khellang Dec 4, 2015

davidfowl Dec 4, 2015

khellang Dec 4, 2015

davidfowl commented Dec 4, 2015


		public FormFile(Stream baseStream, long baseStreamOffset, long length)
		public FormFile(Stream baseStream, long baseStreamOffset, long length, ContentDispositionHeaderValue contentDisposition)

Add Name and FileName to IFormFile #500

Add Name and FileName to IFormFile #500

Conversation

khellang commented Dec 4, 2015

davidfowl commented Dec 4, 2015

khellang commented Dec 4, 2015

davidfowl commented Dec 4, 2015

davidfowl commented Dec 4, 2015

khellang commented Dec 4, 2015

davidfowl commented Dec 4, 2015

khellang commented Dec 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Dec 4, 2015

khellang commented Dec 4, 2015

davidfowl commented Dec 4, 2015

davidfowl commented Dec 4, 2015

khellang commented Dec 4, 2015

khellang commented Dec 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Dec 4, 2015